Add Sort Order Option setting to address #27759 #96214

leilapearson · 2020-04-27T02:55:27Z

This PR fixes #27759 by adding a Sort Order Option setting which can be found just below the existing Sort Order setting under Features -> Explorer .

To make it easier to see the effects of the change and the new settings, I created this repo:

https://github.com/leilapearson/filename-sort-identifier

The results of applying the old code as well as the new code with all combinations of Sort Order Option settings can be found here:

https://docs.google.com/spreadsheets/d/1abeyFsARhdd2llJV6V-ya_li9_dtTIFdJRtaa2Gxqkk/edit?usp=sharing

For the most part, sorts using the Sort Order Option default value of numeric will behave like sorts did before this option existed - but there are a few intentional changes. These are noted in the spreadsheet above, but here is the summary:

The old numeric sort did not compare names separately from extensions. This caused aggregate_repo.go to sort before aggregate.go because when using a locale-based sort the underscore character sorts before all alphabetic characters. By comparing names first and then extensions, the longer name will sort after the shorter name.
The old numeric sort used base sensitivity - but then had a check to see if the strings were unequal and resolved differences using a unicode sort order. This meant that case differences and accent differences were being taken into account, but according to unicode order, not locale order, which should not have been the case. The new code leaves the sensitivity setting alone (which normally defaults to variant) so that things will sort properly according to platform locale - which means it checks first if strings differ in base characters, then if they differ in character accents, and finally if they differ in case, and sorts based on what users would expect.
The old code treated any dotfile that had a leading dot and no other dots in the name as if it was an empty name plus an extension. It treated a dotfile with a leading dot and one or more other dots as if it was a name that started with a dot plus an extension. The new code treats the whole dotfile name as a name that starts with a dot and doesn't have an extension at all. This old behavior caused dotfiles that didn't have a second dot to appear between groups of files sorted by type - since the dotfiles themselves were treated as types. This seemed odd and inconsistent. For example, .eslintrc.json would sort with other .json files in the j section, which seemed like potentially a good thing - but .eslintignore would sort in between .cs files and .fs files - far from .eslintrc.json in the j extensions section. People are more used to seeing all dotfiles together, regardless of extension, so that's what the new code does.

There were quite a few decisions to make in how the various sort algorithms should behave - so I wound up tweaking things a number of times before I settled on the above behavior. I tried to follow the principle of least surprise when choosing between ways of handling things. Hopefully I succeeded, but if not, further tweaks can certainly be made.

CC: @isidorn

msftclas · 2020-04-27T02:55:40Z

All CLA requirements met.

leilapearson · 2020-04-27T02:59:03Z

src/vs/base/common/comparers.ts

+
+// A collator with numeric sorting enabled.
+const intlFileNameCollatorNumeric: IdleValue<{ collator: Intl.Collator }> = new IdleValue(() => {
+	const collator = new Intl.Collator(undefined, { numeric: true });


Removed the sensitivity: base option here. It was doing more harm than good since the code was falling back to a unicode comparison when there were accent or case differences.

src/vs/base/common/comparers.ts

leilapearson · 2020-04-27T03:09:15Z

src/vs/base/common/comparers.ts

 	}

-	return result;
+	// Using the numeric option in the collator will make compare(`foo1`, `foo01`) === 0. Sort the shorter name first.


Old code sorted in unicode order here - and not just if the lengths were different - if there was any kind of difference between the strings including case or accent.

leilapearson · 2020-04-27T03:12:23Z

src/vs/base/common/comparers.ts

+		return oneName.length < otherName.length ? -1 : 1;
+	}
+
+	// Check for case insensitive extension differences, comparing numbers numerically instead of alphabetically.


Extensions are a case where it seems best to use a case-insensitive (but not accent-insensitive) check before disambiguating numbers. Their case only comes into play at the very end if everything else has no difference. This makes sense because .MD and .md are treated as the same thing when determining file type.

leilapearson · 2020-04-27T03:14:26Z

src/vs/base/common/comparers.ts

-	return [(match && match[1]) || '', (match && match[3]) || ''];
+	let result: [string, string] = [(match && match[1]) || '', (match && match[3]) || ''];
+
+	// treat an empty filename with an extension, or a filename that starts with a dot, as a dotfile name


Added this special handling of dotfiles as mentioned in the PR submission comment.

src/vs/base/common/comparers.ts

leilapearson · 2020-04-27T03:24:57Z

src/vs/base/common/comparers.ts

+ *    0 otherwise
+ * ```
+ */
+export function compareCaseLowerFirst(one: string, other: string): number {


I exported these functions so I could test them. They seem simple, but this is actually one of the main areas that I tweaked. I originally wasn't sure whether or not uppercase and lowercase should be considered greater than non-case letters or not, and whether a word that starts with a non-case character like an underscore should be non-case because of that underscore, or should be judged by the first character that has a case.

I can remove the exports and the tests now if desired.

src/vs/base/common/comparers.ts

leilapearson · 2020-04-27T03:28:25Z

src/vs/base/common/comparers.ts

-	// Using the numeric option in the collator will
-	// make compare(`foo1`, `foo01`) === 0. We must disambiguate.
-	if (intlFileNameCollator.getValue().collatorIsNumeric && result === 0 && a !== b) {
-		return a < b ? -1 : 1;


Note the unicode comparison here.

leilapearson · 2020-04-27T03:29:26Z

src/vs/base/common/comparers.ts

-export function compareFileNames(one: string | null, other: string | null, caseSensitive = false): number {
-	const a = one || '';
-	const b = other || '';
-	const result = intlFileNameCollator.getValue().collator.compare(a, b);


Note that unlike the other comparisons, this one wasn't separately comparing names and extensions - which cause problems like the aggregate_repo.go vs aggregate.go issue.

leilapearson · 2020-04-27T03:30:09Z

src/vs/base/common/comparers.ts

 }

-const FileNameMatch = /^(.*?)(\.([^.]*))?$/;


I didn't change this. I just moved it down next to the function that uses it.

leilapearson · 2020-04-27T03:31:52Z

src/vs/base/common/comparers.ts


-		if (intlFileNameCollator.getValue().collatorIsNumeric && result === 0 && oneName !== otherName) {
-			return oneName < otherName ? -1 : 1;


Note the unicode comparison here.

leilapearson · 2020-04-27T03:33:58Z

src/vs/base/common/comparers.ts

 }

-function extractNameAndExtension(str?: string | null): [string, string] {


Hmm... this one is exported in my code too? I'll remove the export.

leilapearson · 2020-04-27T03:38:20Z

src/vs/base/common/comparers.ts

+	return one < other ? -1 : 1;
+}
+
+/** Compares filenames by name unicode value, then extension unicode value. */


This function doesn't seem to be used anywhere except in extHostDocumentData.test.ts. Could it possibly be removed? Or does it need to stick around? Same with compareByPrefix...

leilapearson · 2020-04-27T03:46:15Z

src/vs/base/test/browser/comparers.test.ts

 	});
+
+	test('compareCaseLowerFirst', () => {


I can remove these tests and not export these functions if that's preferred. The compareCase* functions are helper functions. It was good to be able to test them directly during development, but the other tests cover their behavior indirectly.

isidorn · 2020-04-27T07:47:21Z

Thanks for the PR.
This week we are in endgame week which means we are not accepting any large changes, thus assigning this to May and will probably review this in the next two weeks.

Originaly I thought that this PR would only touch changes on the Explorer, however I see there are a lot of changes in the Comparer.ts thus also assigning @bpasero and @jrieken to take a look.

bpasero

The changes in comparers.ts are beyond of what I can review just now. I am a bit worried that this change impacts more than just the explorer (e.g. we use comparers in quick open). I would opt for a solution that keeps the comparers as they work today for anything that is not the explorer where we want to provide the new search options.

bpasero · 2020-04-27T11:48:26Z

src/vs/workbench/contrib/files/browser/files.contribution.ts

@@ -398,6 +398,19 @@ configurationRegistry.registerConfiguration({
 			],
 			'description': nls.localize('sortOrder', "Controls sorting order of files and folders in the explorer.")
 		},
+		'explorer.sortOrderOption': {


How is it possibly to understand the difference between explorer.sortOrder setting and explorer.sortOrderOption?

Sort Order Option further refines the sort order. I was hoping the description made that clear - but can certainly reword if it doesn't.

Maybe "further refines" would be better wording.

bpasero · 2020-04-27T11:51:27Z

src/vs/base/common/comparers.ts

+// Export compareFileNamesNumeric by its old name too for backwards compatibility.
+// Also keeping a placeholder parameter for the same reason. The parameter was sometimes supplied but was
+// not used in the original code.
+export { compareFileNamesNumeric as compareFileNames };


Since we seem to be supporting the old name, why are there changes in this PR to change to compareFileNamesNumeric?

I don't really want to support the old name - but I wasn't sure how to safely change it? If I don't support the old name there are problems in CI building the stand-alone editor.

I've updated the code so that instead of just exporting an alias (which didn't work with tree-shaking) I export a compareFileNames function that calls compareFileNamesNumeric - so now the CI checks pass at least.

Fixes microsoft#94556

…rendere fixes microsoft#95887

- check for active view descriptors - add model tests

Fix code comment block for Pseudoterminal (missing closing backticks)

…ft#96079.

Part of microsoft#90528

leilapearson · 2020-04-27T21:01:37Z

Thanks for the responses @isidorn and @bpasero. Totally understand waiting until May for the full review. It's a bigger change then I was expecting it to be too!

@bpasero I'm totally open to making the old functions - compareFileNames and compareFileExtensions - behave the same way as they did before - and using those functions outside of Explorer if that works best.

Once you do have a chance to review things you can let me know if you think any of the changes I made for compareFileNamesNumeric and compareFileExtensionsNumeric should be ported into compareFileNames and compareFileExtensions or not.

leilapearson · 2020-04-27T21:11:49Z

P.S. I merged the latest changes from master into my fork and into my PR branch - which may not have been the best idea during the review. Just let me know if you'd like me to do something about that - and if so what's the best way to handle it. I did read through your coding guidelines but I'm not too familiar with your preferred workflow. Thanks! :-)

isidorn · 2020-05-06T14:49:51Z

@leilapearson thanks again for this PR. I see you put a lot of work and effort in this PR and we really appreciate it. However there are a couple of issues for us to proceed further with this:

I think you rebased and not merged with latest from master - thus the PR shows it changed 107 files. You should only merge, so the PR changes just the files you touched
As already mentioned this PR touches some areas which affect the whole workbench. It would be great if this PR was more localised and only touching the explorer area
It is much easier for us to review and merge smaller and concise PRs, I am not sure if it is possible to break this one in logical pieces which make sense seperatly.

Thank you again and I appologise if I did not specify this before you started on the implementation.

leilapearson · 2020-05-06T22:16:29Z

Thanks for getting back to me @isidorn.

I think you rebased and not merged with latest from master - thus the PR shows it changed 107 files. You should only merge, so the PR changes just the files you touched

As already mentioned this PR touches some areas which affect the whole workbench. It would be great if this PR was more localised and only touching the explorer area

It is much easier for us to review and merge smaller and concise PRs, I am not sure if it is possible to break this one in logical pieces which make sense separately.

Yeah. I messed it up. Since you've asked for a smaller PR anyway, I'll close this PR up and submit new PRs with clean histories. See (3).
Most of the changes in this PR are due to the new compare functions. Those functions aren't used outside of explorer. However, I did fix a couple of edge cases in the existing numeric sort and updated code outside of explorer to get these fixes too. Since that's a concern, I'll go ahead and change it so that the old functionality is used outside of explorer. If you later decide that these fixes would be good for other parts of workbench too, I can either update the PR or submit another new PR with that change.
I have a couple of ideas on how to break this down into smaller PRs. Can you let me know which you prefer?
a. One PR for the numeric sort algorithm fixes + one PR to add the new sort order options.
b. One PR for the numeric sort algorithm fixes + one PR for each of the new options (upper, lower, mixed, and unicode) - so 5 PRs in total.

Since both options include submitting a PR with just the numeric sort algorithm fixes, I'll go ahead with that part while awaiting your response.

Thanks again @isidorn for the continued support and guidance. I did put a lot of effort into this but I don't at all mind putting in some more to make this easier to review.

isidorn · 2020-05-07T07:45:18Z

@leilapearson let's go with 3a. Thanks!

leilapearson · 2020-05-07T18:08:39Z

Will do @isidorn. Thanks!

leilapearson · 2020-05-08T01:56:06Z

Closing this PR. Per our discussion, I'm splitting this PR into two separate PRs for easier review:

one PR for the numeric sort algorithm fixes + one PR to add the new sort order options.

I've already created PR #97200 for the first part. I will create the second PR soon.

Add Sort Order Option setting to address microsoft#27759

4ffc5c6

leilapearson commented Apr 27, 2020

View reviewed changes

src/vs/base/common/comparers.ts Outdated Show resolved Hide resolved

leilapearson commented Apr 27, 2020

View reviewed changes

src/vs/base/common/comparers.ts Outdated Show resolved Hide resolved

leilapearson commented Apr 27, 2020

View reviewed changes

src/vs/base/common/comparers.ts Outdated Show resolved Hide resolved

leilapearson commented Apr 27, 2020

View reviewed changes

Remove extra exports

928beec

sbatten assigned isidorn Apr 27, 2020

isidorn assigned bpasero and jrieken Apr 27, 2020

isidorn added this to the May 2020 milestone Apr 27, 2020

jrieken removed their assignment Apr 27, 2020

bpasero reviewed Apr 27, 2020

View reviewed changes

bpasero removed their assignment Apr 27, 2020

Tyriar and others added 4 commits April 27, 2020 08:52

Show args in default keybindings

7e7ef75

Fixes microsoft#94556

Prevent unexpected rename cancellation

c5344ab

Use the cancellation token to trigger rename cancellation

5bb2e2c

Make range in EditorStateCancellationTokenSource readonly

31a4db7

jrieken and others added 19 commits April 27, 2020 08:52

💄

2306c34

git: use environmentVariableCollection

2c77e38

git: config.terminalAuthentication

335b1b0

git: config.githubAuthentication

d64f1d2

💄

a3809d3

fix button codicon colors

8677d6c

Fix spacing after when

0a7b38a

CallStack: We need the MenuEntryActionViewItem so the icon would get …

c18e86b

…rendere fixes microsoft#95887

microsoft#96064 Fix model and view

c2cda6e

- check for active view descriptors - add model tests

do not throw error and continue

58e36a5

Update vscode.d.ts

158cfc7

Fix code comment block for Pseudoterminal (missing closing backticks)

Throttle status bar spinner animations to save CPU

f3d8e31

Treat .gitignore_global as an Ignore file by default. Fixes microso…

3662f2b

…ft#96079.

rename "Trigger" to "TriggerKind"; see microsoft#88230

7754ccc

Disable tasks tests in web

47bb655

Part of microsoft#90528

fixes microsoft#95940

058bb93

Add ref to why test is disabled

2025795

fix microsoft#96256

550fc69

restore compareFiles and compareFileExtensions

e471148

isidorn modified the milestones: May 2020, Backlog May 6, 2020

leilapearson closed this May 8, 2020

github-actions bot locked and limited conversation to collaborators Jun 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sort Order Option setting to address #27759 #96214

Add Sort Order Option setting to address #27759 #96214

leilapearson commented Apr 27, 2020

msftclas commented Apr 27, 2020 •

edited

Loading

leilapearson Apr 27, 2020

leilapearson Apr 27, 2020

leilapearson Apr 27, 2020

leilapearson Apr 27, 2020

leilapearson Apr 27, 2020

leilapearson Apr 27, 2020

leilapearson Apr 27, 2020

leilapearson Apr 27, 2020 •

edited

Loading

leilapearson Apr 27, 2020

leilapearson Apr 27, 2020

leilapearson Apr 27, 2020 •

edited

Loading

leilapearson Apr 27, 2020

isidorn commented Apr 27, 2020

bpasero left a comment

bpasero Apr 27, 2020

leilapearson Apr 27, 2020 •

edited

Loading

bpasero Apr 27, 2020

leilapearson Apr 27, 2020

leilapearson commented Apr 27, 2020

leilapearson commented Apr 27, 2020

isidorn commented May 6, 2020

leilapearson commented May 6, 2020 •

edited

Loading

isidorn commented May 7, 2020

leilapearson commented May 7, 2020

leilapearson commented May 8, 2020


		if (intlFileNameCollator.getValue().collatorIsNumeric && result === 0 && oneName !== otherName) {
		return oneName < otherName ? -1 : 1;

		}

		function extractNameAndExtension(str?: string \| null): [string, string] {

Add Sort Order Option setting to address #27759 #96214

Add Sort Order Option setting to address #27759 #96214

Conversation

leilapearson commented Apr 27, 2020

msftclas commented Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leilapearson Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leilapearson Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidorn commented Apr 27, 2020

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leilapearson Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leilapearson commented Apr 27, 2020

leilapearson commented Apr 27, 2020

isidorn commented May 6, 2020

leilapearson commented May 6, 2020 • edited Loading

isidorn commented May 7, 2020

leilapearson commented May 7, 2020

leilapearson commented May 8, 2020

msftclas commented Apr 27, 2020 •

edited

Loading

leilapearson Apr 27, 2020 •

edited

Loading

leilapearson Apr 27, 2020 •

edited

Loading

leilapearson Apr 27, 2020 •

edited

Loading

leilapearson commented May 6, 2020 •

edited

Loading